Skip to content

src: don't kill own process group on failed spawn - #65054

Open
lazerg wants to merge 3 commits into
nodejs:mainfrom
lazerg:fix/issue-65052-kill-unspawned-process
Open

src: don't kill own process group on failed spawn#65054
lazerg wants to merge 3 commits into
nodejs:mainfrom
lazerg:fix/issue-65052-kill-unspawned-process

Conversation

@lazerg

@lazerg lazerg commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

libuv only assigns a pid to the process handle once uv_spawn() succeeds, so a child that never started keeps pid 0. Calling kill() on that child still reached uv_process_kill(), which ended up in kill(0, signal) and signalled every process in the caller's own process group, Node included. The handle is now reported as ESRCH when there is no pid, so child.kill() just returns false. The pid is also zeroed in the constructor so the check never reads an unassigned value.

Reproducing it on its own needs no prototype tampering:

const { spawn } = require('child_process');
const child = spawn('foo123');
child.on('error', () => {});
child.kill();  // terminates the whole process group

In the report the spawn failed for a different reason: overriding Array.prototype[Symbol.iterator] makes normalizeSpawnArguments() build an empty env, so the command was no longer found through PATH. The dead handle is what took the parent down.

Behavior change

kill() on a child that failed to spawn used to return true on POSIX. It now returns false.

Windows was never affected by the process-group problem, since uv_process_kill() bails out with EINVAL when the handle has no process handle. It reached the throw new ErrnoException(err, 'kill') branch instead, so there kill() goes from throwing EINVAL to returning false. Both platforms now agree.

subprocess.killed is also left alone in this case, which matches what the docs already say about it being set only when a signal is sent successfully.

Fixes: #65052

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Aug 5, 2026
libuv only assigns a pid to the process handle once uv_spawn() has
succeeded, so a child that never started keeps pid 0. Calling kill()
on such a child still reached uv_process_kill(), which ended up in
kill(0, signal) and signalled every process in the caller's own
process group, Node included.

Return ESRCH when the handle has no pid, and zero the pid in the
constructor so the check never reads an unassigned value.

Signed-off-by: Lazizbek Ergashev <lazerg2@gmail.com>
@lazerg
lazerg force-pushed the fix/issue-65052-kill-unspawned-process branch from 1c77865 to 38627f1 Compare August 5, 2026 16:51
Comment thread src/process_wrap.cc
Comment thread test/parallel/test-child-process-kill-spawn-error.js Outdated
@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 21, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 21, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.30%. Comparing base (dc7838c) to head (3174c46).
⚠️ Report is 257 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65054      +/-   ##
==========================================
- Coverage   90.30%   90.30%   -0.01%     
==========================================
  Files         759      759              
  Lines      247629   247630       +1     
  Branches    46688    46696       +8     
==========================================
- Hits       223618   223615       -3     
- Misses      15469    15472       +3     
- Partials     8542     8543       +1     
Files with missing lines Coverage Δ
src/process_wrap.cc 75.11% <100.00%> (+0.11%) ⬆️

... and 19 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SIGTERM killing an invalid node:child_process spawn terminates main process

4 participants